-
Notifications
You must be signed in to change notification settings - Fork 216
Improve caching #1375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve caching #1375
Conversation
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
|
/review |
simonferquel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small changes to make things a tiny bit more robust
| } | ||
| } | ||
|
|
||
| // Cache control checkpoint #1 out of 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit brittle to only rely on where you add messages in the « system slices » compared to that line of code to make it part of what we think is invariant or not. It is a bit too easy to break the cache.
Can we separate the messages into different slices (invariant messages, lowFrequencyChangeMessages, …) and put the checkpoint when we merge them (in correct order) in the system messages slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonferquel Can we merge this and you prepare another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
simonferquel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking, fixing in followup
Added a few tests and e2e on the way.